Automated review 🤖

Summary of Changes

This PR brings in operational fixes aimed at reducing memory growth and improving observability: capped in-memory MCP server logs, periodic process/resource stats logging, and explicit Puppeteer browser shutdown in the built-in Searxng server. The overall direction is good and aligned with the stated OOM/tab-leak concerns, but there is one notable performance concern in the log-cap implementation under sustained stderr/error volume.

Key Changes & Positives

• 🟢 (Major) src/builtin-servers/servers/searxng.ts now calls await this.scraper.closeBrowser() during stop(), and resets scraper/scraperReady, which is the right lifecycle cleanup for the browser-backed server.
• 🟢 (Major) src/index.ts wires in ResourceStatsService behind RESOURCE_STATS_INTERVAL_MS, with 0 disabling sampling via src/env.ts; this is a sensible operational toggle.
• 🟢 (Minor) src/utils/resourceStats.ts isolates the private _process.pid access in one helper (getStdioTransportPid), which limits SDK-internals coupling.
• 🟢 (Minor) Added focused tests for log capping and ps parsing/aggregation in test/server-logs-cap.spec.ts and test/resource-stats.spec.ts.

Potential Issues & Recommendations

1. • Issue / Risk: appendServerLog() in src/services/mcp.ts:934 uses logs.splice(0, logs.length - MAX_SERVER_LOG_LINES) on every append past the cap, which is O(n) and shifts the entire array.
   • Impact: Under the exact high-volume stderr/error scenarios this PR addresses, this can add avoidable CPU overhead and GC churn while the process is already stressed.
   • Recommendation: Replace with a constant-size ring buffer, or in the minimal case use if (logs.length >= MAX_SERVER_LOG_LINES) logs.shift() before push(); ideally add a micro-benchmark or stress test around repeated appends.
   • Status: 🟡 Needs review
2. • Issue / Risk: sampleProcessTree() in src/utils/resourceStats.ts:105 depends on ps -eo pid=,ppid=,rss=,pcpu=,comm= and assumes compatible output semantics across runtime environments.
   • Impact: Container/minimal images or nonstandard ps implementations may silently reduce visibility to self-only stats after the first warning.
   • Recommendation: Keep the current fallback, but add a test or documented runtime requirement for the deployment image; verify in the production container with ps -eo pid=,ppid=,rss=,pcpu=,comm=.
   • Status: 🟡 Needs review

Language/Framework Checks

• TypeScript/Node:
  • Logging/error handling looks safe; async interval callback catches failures in src/services/resourceStats.ts.
  • Env parsing in src/env.ts:22 is permissive; invalid numeric strings collapse to 0 and disable stats. That may be intentional, but is worth confirming.
  • Private SDK internals access is runtime-guarded and contained, which is the safest available pattern here.
  • No obvious ESM/CJS or syntax issues in the added files.

Security & Privacy

• Resource-stat logs include process names/commands via comm; this is usually low risk, but verify command names do not expose sensitive arguments in your target environments.

Build/CI & Ops

• Operationally, RESOURCE_STATS_INTERVAL_MS changes runtime logging volume and should be called out for rollout/rollback: set to 0 to disable if logs become noisy or sampling is problematic in a given container image.

Tests

• Current coverage is good for the new helpers.
• Add a stress-style unit test or benchmark for appendServerLog() behavior at sustained volume to validate the chosen cap implementation under load.

Approval Recommendation

Approve with caveats

• Revisit appendServerLog() to avoid repeated full-array shifts/splices under high log volume.
• Verify ps availability/compatibility in the actual deployment container or document it as an environment requirement.

Addressed the review feedback in b1200d0 (v1.11.13). Point by point:

Copilot

• mcp.ts JSDoc duplicated "per" → reworded to "for each MCP server and each per-user connection".
• resourceStats service import { Resource } → changed to import type { Resource } (Resource is a type-only export; avoids a runtime circular import with ../index).
• resourceStats util comm= vs command= → switched to command= and added shortenProcessLabel(), which derives a concise label (executable basename, plus the script basename for node/python/etc. so sibling processes stay distinct). This also drops all command-line args from the label, so the chrome/node labels stay clean and no argument values can reach the logs.

Automated reviewer

• appendServerLog O(n) concern: kept the strict last-500 semantics (an explicit product requirement, and the suggested shift() is the same O(n) cost as the current single-element splice; a true ring buffer would break the ordered-string[] contract that GET /mcp/servers and the close-handler serialization depend on). Added a 100k-append sustained-volume stress test proving the buffer never exceeds 500 at any point and retains the newest lines.
• ps availability across images: added procps to the Dockerfile so ps is guaranteed in the runtime image; the sampler already warns-once and falls back to self-only stats if it's ever missing.
• Env parsing: RESOURCE_STATS_INTERVAL_MS now defaults to 60000 on unset/blank/NaN (a typo no longer silently disables sampling); an explicit 0 still disables.
• Security (command names): labels now contain only executable/script basenames, never full args.

tsc --noEmit clean; full jest suite 177/177 (added shortenProcessLabel coverage incl. an arg-non-leakage assertion, and the stress test).

Automated review 🤖

Summary of Changes

This PR adds two operational safeguards: bounded in-memory MCP server logs to prevent unbounded growth, and periodic resource usage logging for the API process plus spawned child-process subtrees. It also updates Puppeteer scraper lifecycle handling to explicitly close the browser and bumps the scraper dependency to pick up an upstream Chromium tab leak fix.

Key Changes & Positives

• 🟢 (Major) src/services/mcp.ts introduces appendServerLog() with a hard cap of 500 retained lines, replacing unbounded .push() paths for server and connection log buffers.
• 🟢 (Major) src/services/resourceStats.ts and src/utils/resourceStats.ts add periodic self/child process resource sampling with explicit disable support via RESOURCE_STATS_INTERVAL_MS <= 0.
• 🟢 (Major) src/builtin-servers/servers/searxng.ts now calls await this.scraper.closeBrowser() during shutdown and clears local scraper state afterward.
• 🟢 (Minor) Dockerfile adds procps, which aligns with the new ps-based process-tree sampling on Linux containers.
• 🟢 Tests were added for the new log-capping and resource-stat parsing utilities, including negative cases and high-volume append behavior.

Potential Issues & Recommendations

1. Issue / Risk: src/utils/resourceStats.ts parses ps output by splitting on whitespace, which is fragile if ps emits locale-specific CPU formatting or unexpected command header/format variations across environments.
   Impact: Resource-stat logging could silently skip processes or under-report child usage on some hosts.
   Recommendation: Keep the current parser, but add a CI test fixture for representative Linux and macOS ps output formats, and verify in container/runtime with ps -eo pid=,ppid=,rss=,pcpu=,command=.
   Status: 🟡 Needs review

2. Issue / Risk: src/services/resourceStats.ts logs sampling failures at debug inside the interval callback after an initial warn path only covers sampleProcessTree().
   Impact: Repeated failures outside the ps call path could become invisible in production if debug logging is disabled.
   Recommendation: Consider rate-limited warn logging for repeated logSample() failures, or separate handling for CPU/memory/self vs subtree sampling errors.
   Status: 🟡 Needs review

3. Issue / Risk: src/services/mcp.ts:929 uses splice(0, logs.length - MAX_SERVER_LOG_LINES) on every append after the cap is reached.
   Impact: For very chatty stderr streams, this is O(n) per append and could add avoidable overhead, even though the cap is small.
   Recommendation: Acceptable as-is for 500 entries, but if volume is high in practice, switch to a ring buffer/deque implementation.
   Status: 🟢 Correct

Language/Framework Checks

• TypeScript/Node:

  • RESOURCE_STATS_INTERVAL_MS parsing in src/env.ts is safe and preserves explicit 0 disable semantics. 🟢
  • Private-field access to StdioClientTransport._process is intentionally isolated in src/utils/resourceStats.ts; good containment, but this should be revalidated on SDK upgrades. 🟡
  • Async interval callback in ResourceStatsService.init() catches rejections correctly, avoiding unhandled promise rejections. 🟢
  • Runtime dependency change in package.json/yarn.lock is consistent.

• Docker:

  • Dockerfile adds the required procps package for ps; dependency aligns with runtime behavior. 🟢
  • No .dockerignore context shown, so not enough context to assess build-context impact.

Build/CI & Ops

• RESOURCE_STATS_INTERVAL_MS changes runtime logging behavior; rollout should verify expected log volume and confirm 0 disables sampling cleanly.
• Child-process stats depend on ps availability; container path is covered by procps, but non-container environments should be verified operationally.

Tests

• Added unit coverage for appendServerLog, parsePsOutput, aggregateDirectChildSubtrees, shortenProcessLabel, and getStdioTransportPid. 🟢
• Consider adding one integration test around BuiltInSearxngServer.stop() to verify closeBrowser() is invoked and scraper state resets after shutdown.

Approval Recommendation

Approve with caveats

• Verify ps parsing against actual Linux and macOS output in CI or a fixture-based test.
• Decide whether repeated resource-sampling failures should surface above debug level in production.

Addressed round-2 feedback in f6c398d (v1.11.14):

• shortenProcessLabel arg leak (resourceStats.ts): eval/print code after -e/-c was being picked up as the "script". It now only appends a token that looks like a script path (path separator or known extension), so inline code and other arg values never reach the label. Added tests incl. node -e SECRET → node.
• Sampler overlap (resourceStats service): the setInterval now skips a tick while a previous logSample() is still running (a sampling flag), so a slow/blocked ps can't pile up concurrent samples.
• Retry-after-stop leak (searxng): stop() now cancels the pending init-retry timer and sets a stopped flag; the retry loop bails when stopped and closes a browser that finished initializing after stop(). New mocked searxng-lifecycle suite covers retry scheduling, cancellation on stop, and the mid-init close race.

tsc --noEmit clean; full jest suite 182/182 across 9 suites.

Automated review 🤖

Summary of Changes

This PR adds periodic resource-usage logging, caps in-memory MCP server/connection logs to prevent unbounded growth, and hardens the SearxNG/Puppeteer lifecycle to avoid browser/tab leaks during retries and shutdown. Overall, the changes are targeted at memory stability and observability, with appropriate dependency and container updates to support the new process-tree sampling.

Key Changes & Positives

• 🟢 (Major) src/services/resourceStats.ts introduces a dedicated ResourceStatsService with interval-based sampling, unref() on timers, and a guard against overlapping samples.
• 🟢 (Major) src/services/mcp.ts now caps retained log lines via appendServerLog(...), addressing the core unbounded-array growth path for stderr/transport errors.
• 🟢 (Major) src/builtin-servers/servers/searxng.ts closes browsers on stop(), cancels pending retry timers, and safely handles the race where stop() occurs during async scraper initialization.
• 🟢 (Minor) Dockerfile adds procps, which is necessary for ps-based subtree sampling in Linux containers.
• 🟢 Tests cover the main risk areas: log capping, process-label parsing, process-tree sampling, and Puppeteer retry/stop lifecycle.

Potential Issues & Recommendations

1. Issue / Risk: src/services/mcp.ts:3846 getStdioServerPids() returns entries keyed by server.name, which may not be unique across installed/running stdio servers.
   Impact: Resource-stat log lines may ambiguously attribute memory/CPU usage when multiple child processes share the same display name.
   Recommendation: Use a stable unique label in the tracked map (for example serverKey or server.name plus pid/config identifier) so child logs are unambiguous.
   Status: 🟡 Needs review

2. Issue / Risk: src/utils/resourceStats.ts:149 hard-depends on parsing ps -eo pid=,ppid=,rss=,pcpu=,command= output, which is platform/tooling-sensitive and only lightly validated.
   Impact: On environments where ps output differs, child/total metrics may silently degrade to self-only stats.
   Recommendation: Keep the current fallback behavior, but add an assertion-style test for the exact ps invocation format used in CI/container images, or document Linux/macOS support explicitly near RESOURCE_STATS_INTERVAL_MS.
   Status: 🟡 Needs review

3. Issue / Risk: src/utils/resourceStats.ts:130 tokenizes commands with split(/\s+/), which cannot respect quoted script paths.
   Impact: Labels may become less accurate for commands with spaces in executable/script paths, reducing observability quality.
   Recommendation: If such paths are expected, switch to a shell-aware tokenizer or keep current behavior but document that labels are best-effort only.
   Status: 🟢 Correct

Language/Framework Checks

• TypeScript/Node:
  • Strict typing is generally solid; import type { Resource } is correctly used in src/services/resourceStats.ts.
  • Async error handling is sound in sampling and Puppeteer init/stop flows.
  • Runtime validation for RESOURCE_STATS_INTERVAL_MS in src/env.ts is appropriate and handles blank/NaN safely.
  • Accessing StdioClientTransport private internals in src/utils/resourceStats.ts:29 is intentionally isolated and runtime-guarded, which is the right containment for SDK fragility.

Build/CI & Ops

• Dockerfile:55 aligns the runtime image with the new ps dependency, avoiding container-only breakage.
• The new sampling interval changes operational log volume; verify default 60000ms is acceptable for production log retention and cost.

Tests

• Added coverage is strong and focused.
• Consider one additional integration-style test around ResourceStatsService to verify that when sampleProcessTree() throws repeatedly, the warning is emitted once and sampling continues for self stats without crashing.

Approval Recommendation

Approve with caveats

• Consider making resource-stat child labels unique in src/services/mcp.ts:getStdioServerPids()
• Confirm/document expected ps compatibility for deployed environments

Addressed round-3 feedback in ea3167c (v1.11.15):

• shortenProcessLabel flag-value parsing — node -r ts-node/register /app/index.js was logging node register; the helper now parses flags and their values, skips preloaded-module values (-r/--require/--import/--loader), and never surfaces inline eval code (-e/--eval/-p/--print/-c) even when it contains a / or a script-like extension. Real script is correctly selected → node index.js.
• formatMebibytes unit — math is base-1024 but rendered MB; now renders MiB (matches docker stats/top).
• Log-cap stress-test name — renamed from "memory does not grow" to accurately state it asserts the array length never exceeds the cap.

Added tests for -r/--require/--import value-skipping and eval code containing slashes/extensions (asserting no leak). tsc --noEmit clean; full jest suite 184/184 across 9 suites.

Automated review 🤖

Summary of Changes

This PR adds two production-facing safeguards: bounded in-memory server logs to prevent unbounded growth, and periodic resource-stat logging to make process/subprocess memory and CPU usage visible. It also tightens the SearxNG/Puppeteer lifecycle so shutdown and failed initialization paths actively close browsers and stop retry timers, which should materially reduce the OOM/tab-leak behavior described.

Key Changes & Positives

• 🟢 (Major) src/services/mcp.ts introduces appendServerLog() with a hard cap of 500 retained lines, replacing unbounded .push() calls on both server and connection log buffers.
• 🟢 (Major) src/services/resourceStats.ts cleanly encapsulates periodic sampling and avoids overlapping runs with the sampling guard; unref() on timers is also the right operational choice.
• 🟢 (Major) src/builtin-servers/servers/searxng.ts now closes the browser in stop(), clears pending retry timers, and handles the “stop during init” race by closing a newly created scraper before publishing it.
• 🟢 (Minor) Dockerfile adds procps, which matches the new runtime dependency on ps.
• 🟢 Tests cover the new utility parsing, lifecycle race handling, and log-cap behavior in a focused way.

Potential Issues & Recommendations

1. • Issue / Risk: appendServerLog() uses logs.splice(0, logs.length - MAX_SERVER_LOG_LINES) on every append past the cap in src/services/mcp.ts:939.
   • Impact: Under very chatty stderr streams this is O(n) per line and can itself become a CPU hotspot even though memory is bounded.
   • Recommendation: If these buffers are expected to be high-volume in production, switch to a fixed-size ring buffer or trim in larger batches; otherwise add a perf note and monitor CPU under load.
   • Status: 🟡 Needs review
2. • Issue / Risk: shortenProcessLabel() tokenizes ps command= with trimmed.split(/\s+/) in src/utils/resourceStats.ts:139, which does not preserve shell quoting.
   • Impact: Labels may be inaccurate for commands with quoted paths/arguments, and the heuristic could misidentify the script token in edge cases.
   • Recommendation: If log accuracy matters for more than best-effort observability, document this limitation or parse /proc on Linux; otherwise keep as-is and treat labels as advisory only.
   • Status: 🟡 Needs review
3. • Issue / Risk: Resource sampling failure is only logged once in src/services/resourceStats.ts:100.
   • Impact: If ps intermittently fails after initially working, operators may lose child-process visibility without recurring signal in logs.
   • Recommendation: Consider logging again on recovery/failure transitions, or emit a periodic warn after N consecutive failures.
   • Status: 🟡 Needs review

Language/Framework Checks

• TypeScript/Node:

  • Typing looks sound overall; import type { Resource } in src/services/resourceStats.ts is correct.
  • Async error handling is reasonable: interval callback catches and suppresses sampling failures without crashing the process.
  • Runtime env parsing in src/env.ts is improved; invalid/blank RESOURCE_STATS_INTERVAL_MS now falls back safely instead of silently disabling.
  • The private _process access in src/utils/resourceStats.ts:31 is intentionally guarded, but it remains coupled to @modelcontextprotocol/sdk internals and should be revalidated on SDK upgrades.

• Docker:

  • Runtime package addition (procps) aligns with the ps dependency introduced by process-tree sampling.
  • No obvious layer/cache regressions beyond the extra apt package.

Security & Privacy

• shortenProcessLabel() is clearly trying to avoid leaking inline eval code and argument values into logs, which is a good safeguard.
• Child-process command sampling still inspects full command lines via ps; while only reduced labels are emitted, verify no raw command strings are logged elsewhere on error paths.

Tests

• Added tests are appropriate and cover happy paths plus key race/negative cases.
• Consider adding one integration-style test for ResourceStatsService that mocks sampleProcessTree() and verifies logging/skip-on-overlap behavior.

Approval Recommendation

Approve with caveats

• Confirm the O(n) trimming in appendServerLog() is acceptable for expected stderr volume, or replace it with a ring buffer.
• Accept/document that shortenProcessLabel() is best-effort and not a shell-accurate parser.

Addressed round-4 feedback in 4972dcb (v1.11.16):

• Shutdown order — ResourceStatsService is now registered at the front of the resources list (unshift), so shutDown() stops it first. Sampling ceases before the MCP/package services it samples begin tearing down, so a tick during SIGTERM can no longer spawn ps or log stale stats mid-shutdown.

tsc --noEmit clean; full jest suite 184/184 across 9 suites.

Automated review 🤖

Summary of Changes

This PR merges the external-oauth branch work into main and primarily addresses two operational issues: unbounded in-memory log growth and Puppeteer/Chromium lifecycle leaks, while adding periodic process/resource visibility for the API and spawned child processes. Overall, the changes are directionally strong for memory stability and diagnosability, with one notable performance concern in the log-capping implementation under sustained high-volume logging.

Key Changes & Positives

• 🟢 (Major) src/builtin-servers/servers/searxng.ts now properly cancels pending retry timers and closes the browser on stop(), including the mid-initialization case; this directly addresses the tab/browser leak risk.
• 🟢 (Major) package.json and yarn.lock bump @missionsquad/puppeteer-scraper to ^1.1.2, which aligns with the stated failed-scrape leak fix.
• 🟢 (Minor) src/index.ts:94 registers ResourceStatsService first in shutdown order, which is a sensible guard against sampling during teardown.
• 🟢 (Minor) src/env.ts adds safer parsing for RESOURCE_STATS_INTERVAL_MS, avoiding typo-driven accidental disablement.
• 🟢 (Minor) src/utils/resourceStats.ts is careful about not leaking eval/preload arguments into logs when deriving process labels.

Potential Issues & Recommendations

1. • Issue / Risk: appendServerLog() in src/services/mcp.ts:936 uses logs.splice(0, logs.length - MAX_SERVER_LOG_LINES) on every append past the cap, which is O(n) and shifts the array each time.
   • Impact: Under exactly the “chatty stderr” scenario this PR targets, CPU time and GC pressure may become significant even though retained memory is bounded.
   • Recommendation: Replace the array-with-front-splice approach with a ring buffer/deque, or at minimum trim in larger batches instead of one line at a time; add a micro-benchmark or perf-oriented test around sustained logging.
   • Status: 🟡 Needs review
2. • Issue / Risk: sampleProcessTree() in src/utils/resourceStats.ts:182 depends on parsing ps -eo pid=,ppid=,rss=,pcpu=,command= output with a regex that assumes numeric rss/pcpu formatting.
   • Impact: This may be somewhat brittle across base image/OS variants or locale settings, causing child-process stats to silently disappear after the one-time warning.
   • Recommendation: Verify in the runtime container and CI image with ps -eo pid=,ppid=,rss=,pcpu=,command=; if portability matters beyond current targets, consider forcing a stable locale or documenting Linux/macOS support explicitly.
   • Status: 🟡 Needs review

Language/Framework Checks

• TypeScript/Node:

  • Typed interfaces and helper extraction are clean; private-field access in getStdioTransportPid() is isolated and runtime-guarded. 🟢
  • Async shutdown handling in searxng.ts looks correct; retry timer unref() is appropriate to avoid holding the process open. 🟢
  • Runtime validation is minimal but acceptable for RESOURCE_STATS_INTERVAL_MS; non-finite values fall back safely. 🟢
  • Not enough context: verify PuppeteerScraper.closeBrowser() is idempotent and safe after partial init by checking its implementation or running the new lifecycle tests plus an integration stop/start cycle.

• Docker:

  • Dockerfile:55 adds procps, which matches the new runtime dependency on ps. 🟢
  • Slight image-size increase is expected; acceptable given the operational feature.

Build/CI & Ops

• Adding ResourceStatsService changes runtime log volume and may affect log ingestion costs/noise, especially at lower intervals.
• RESOURCE_STATS_INTERVAL_MS=0 provides a clean rollback/disable path for the new sampling behavior.

Tests

• Good coverage added for resource-stat parsing/labeling, searxng lifecycle, and log capping. 🟢
• Consider adding a targeted performance test or benchmark for appendServerLog() under sustained >100k append workloads to validate the chosen structure.

Approval Recommendation

Approve with caveats

• Review/consider optimizing src/services/mcp.ts:936 log-cap implementation to avoid repeated front-splice cost under heavy log volume.
• Verify ps parsing behavior in the actual production container/runtime environment.

Addressed round-5 feedback in 0205311 (v1.11.17):

• stop() now awaits an in-flight sample — round-4 stopped the service first but stop() only cleared the interval; a sample already launched (the interval callback doesn't await logSample()) could still spawn ps/log during teardown. The service now tracks the in-flight sampling promise and stop() awaits it, so sampling fully completes before the sampled services tear down.

Added a ResourceStatsService suite (mocked sampleProcessTree) asserting stop() waits for an in-flight sample, returns immediately when idle, and never samples when disabled. tsc --noEmit clean; full jest suite 187/187 across 10 suites.

Automated review 🤖

Summary of Changes

This PR adds periodic resource-usage logging for the API process and its child process subtrees, caps retained in-memory MCP logs to prevent unbounded growth, and hardens the SearxNG/Puppeteer lifecycle to avoid browser/tab leaks during retries and shutdown. Overall, the intent is solid and directly targets known memory/OOM failure modes.

Key Changes & Positives

• 🟢 (Major) src/services/resourceStats.ts introduces a dedicated ResourceStatsService with overlap protection, unref() timers, and shutdown coordination.
• 🟢 (Major) src/services/mcp.ts centralizes log retention via appendServerLog() and enforces a fixed cap of 500 lines for server/connection logs.
• 🟢 (Major) src/builtin-servers/servers/searxng.ts now cancels retry timers and awaits closeBrowser() during shutdown, including the mid-init race where stop() runs before init() finishes.
• 🟢 (Minor) Dockerfile adds procps, which matches the new reliance on ps for process-tree sampling.
• 🟢 Tests cover the main risk areas: retry/shutdown races, process-label parsing, process-tree sampling utilities, and log-cap behavior.

Potential Issues & Recommendations

1. Issue / Risk: appendServerLog() uses Array.splice(0, ...) on every append past the cap in src/services/mcp.ts:938, which is O(n) per log line.
   Impact: A very chatty stderr stream could still create avoidable CPU churn even though memory is bounded.
   Recommendation: Consider a ring buffer or if (logs.length === MAX_SERVER_LOG_LINES) logs.shift() pattern at minimum; ideally encapsulate a fixed-cap queue structure if high-volume logging is expected.
   Status: 🟡 Needs review

2. Issue / Risk: shortenProcessLabel() in src/utils/resourceStats.ts:139 tokenizes command with split(/\s+/), which does not preserve shell quoting/escaping.
   Impact: Labels may be inaccurate for commands containing quoted paths or arguments with spaces, reducing operator usefulness of the resource logs.
   Recommendation: If accuracy matters, prefer sampling executable/args from a source that preserves argv boundaries, or document this as best-effort and verify against representative ps output in Linux/macOS.
   Status: 🟡 Needs review

3. Issue / Risk: getStdioTransportPid() in src/utils/resourceStats.ts:29 depends on the private _process field of @modelcontextprotocol/sdk.
   Impact: A future SDK update could silently stop child-process attribution in resource logs.
   Recommendation: Pin and verify against the SDK version in CI, or add a targeted regression test around a real started StdioClientTransport if feasible.
   Status: 🟡 Needs review

Language/Framework Checks

• TypeScript/Node:
  • Typing is generally good; import type { Resource } in src/services/resourceStats.ts is correct. 🟢
  • Async shutdown paths in src/builtin-servers/servers/searxng.ts and src/services/resourceStats.ts properly await in-flight work. 🟢
  • Env parsing in src/env.ts is safer than raw Number(...), especially preserving explicit 0 as disable. 🟢
  • Runtime dependency on ps is accounted for in Docker, but local/non-Docker environments should still be validated. 🟡

Build/CI & Ops

• The new RESOURCE_STATS_INTERVAL_MS env var changes runtime observability behavior; default 60000 seems reasonable, and 0 provides a clean rollback/disable path.
• Logging volume will increase in production proportionally to child process count and sample interval; verify log-cost/retention settings in deployment targets.

Tests

• Good coverage was added for the new logic.
• I would add one integration-style test for BuiltInSearxngServer.stop() when closeBrowser() rejects, to confirm scraper state is still cleared and shutdown remains resilient.

Approval Recommendation

Approve with caveats

• Consider whether appendServerLog() needs a more efficient bounded buffer under sustained high-volume stderr output.
• Verify the ps command= parsing behavior against real Linux/macOS commands with quoted paths/spaces.
• Confirm the private _process access remains valid for the pinned MCP SDK version and won’t regress unnoticed.

Addressed round-6 feedback in bc56c94 (v1.11.18):

• Init-failure browser close (searxng): the scraper handle is hoisted so the catch closes a partially-created browser before retrying — repeated init failures can no longer leak Chromium (relying on PuppeteerScraper.init()'s internal cleanup alone was fragile). closeBrowser() is a no-op if already cleaned up.
• ps timeout (resourceStats): sampleProcessTree now runs ps with a 5s timeout, so a wedged ps can't stall sampling or (via stop() awaiting the in-flight sample) hang shutdown.
• Disable message (ResourceStatsService): now reports the offending value and states the interval must be a positive number of ms (the guard rejects non-finite values too, not just <= 0).

Added tests: searxng closes the browser on init failure; the service is disabled for a non-finite interval. tsc --noEmit clean; full jest suite 189/189 across 10 suites.

Automated review 🤖

Summary of Changes

This PR brings in three operational fixes: bounded in-memory MCP log retention, periodic resource-usage logging for the API and spawned child process trees, and a Puppeteer/Chromium lifecycle fix intended to stop browser/tab leaks. Overall, the changes are practical and well-tested, with the main impact being improved memory stability and observability in production.

Key Changes & Positives

• (Major) 🟢 src/builtin-servers/servers/searxng.ts now explicitly closes the Puppeteer browser in stop() and on partial init failure, which directly addresses the highest-risk leak path.
• (Major) 🟢 src/services/resourceStats.ts adds a guarded, non-overlapping sampler with unref() and shutdown coordination; this is a solid approach for background telemetry.
• (Minor) 🟢 src/services/mcp.ts centralizes log capping via appendServerLog() and enforces a fixed upper bound of 500 retained lines.
• (Minor) 🟢 Dockerfile adds procps, which aligns with the new ps dependency in src/utils/resourceStats.ts.
• (Minor) 🟢 Test coverage is meaningfully expanded for retry/shutdown edge cases, log-cap behavior, and process-label sanitization.

Potential Issues & Recommendations

1. • Issue / Risk: src/utils/resourceStats.ts:137 tokenizes ps command= using split(/\s+/), which will misparse quoted arguments and some command lines with embedded whitespace.
   • Impact: Logged child labels may be incorrect or fall back unexpectedly, reducing the usefulness of resource attribution and potentially skipping the real script name.
   • Recommendation: If precise labeling matters, prefer ps -o comm= for executable name only, or document that shortenProcessLabel() is best-effort and add tests for quoted command lines to lock in expected behavior.
   • Status: 🟡 Needs review
2. • Issue / Risk: src/services/mcp.ts:933 uses logs.splice(0, logs.length - MAX_SERVER_LOG_LINES) on every append past the cap, which is O(n) per log line.
   • Impact: A very noisy stderr stream could still create avoidable CPU churn even though memory is bounded.
   • Recommendation: Consider a ring buffer or drop exactly one oldest entry when already at capacity (if (logs.length === MAX_SERVER_LOG_LINES) logs.shift() before push) and benchmark under sustained high-volume logging.
   • Status: 🟡 Needs review
3. • Issue / Risk: src/services/resourceStats.ts:111 logs only RSS/proc totals for descendants, not aggregate child CPU in the total line.
   • Impact: Operators may misread the “total” line as a full resource summary when CPU remains fragmented across per-child entries.
   • Recommendation: Either add aggregate child CPU to the total log line or clarify in the message that the total is RSS/process-count only.
   • Status: 🟢 Correct

Language/Framework Checks

• TypeScript/Node:

  • Typing is generally sound; import type usage in src/services/resourceStats.ts:1 is correct. 🟢
  • Runtime validation for RESOURCE_STATS_INTERVAL_MS in src/env.ts:9 is sensible and avoids accidental disablement on malformed env values. 🟢
  • Async shutdown handling in src/services/resourceStats.ts:71 and src/builtin-servers/servers/searxng.ts:102 is correctly awaited. 🟢
  • Private-field reach-in at src/utils/resourceStats.ts:31 is inherently brittle across SDK upgrades; keep the tests as an upgrade guard. 🟡

• Docker:

  • Dockerfile:55 correctly adds procps to support runtime ps sampling. 🟢
  • No other Docker regression is evident from the diff; build context/layering unchanged. 🟢

Security & Privacy

• src/utils/resourceStats.ts:117 makes a good effort to avoid leaking inline eval code or flag values into logs when deriving process labels. 🟢
• The process-label logic is still best-effort parsing of command lines; if child processes may include secrets in unusual positional args, verify emitted [resource-stats] child name=... lines in a staging run. 🟡

Build/CI & Ops

• This introduces a new runtime dependency on ps; the Docker image is updated, but non-container/local deployments should verify ps availability or expect self-only stats with a warning.
• RESOURCE_STATS_INTERVAL_MS=0 provides a clean rollback/disable path for telemetry if logging volume or ps behavior is problematic.
• Startup/shutdown ordering in src/index.ts:97 is thoughtfully handled by registering ResourceStatsService first for teardown. 🟢

Tests

• Added tests are relevant and cover the highest-risk lifecycle paths well.
• Consider adding one more unit test around shortenProcessLabel() for quoted arguments or paths with spaces to document current behavior.

Approval Recommendation

Approve with caveats

• Confirm the shortenProcessLabel() best-effort parsing is acceptable for your production command lines, or add a test covering quoted/path-with-space cases.
• Consider whether the current array-trimming approach in appendServerLog() is acceptable under very noisy stderr workloads.

Addressed round-7 feedback in bad88d7 (v1.11.19):

• getStdioServerPids stale-pid guard — now only maps servers with status connected. Since connection isn't cleared on transport close, a disconnected/error server could otherwise contribute a stale pid and mislabel an unrelated process if the OS reused it. (ps sampling still counts every process; this only governs which subtrees get the friendly server name.)

tsc --noEmit clean; full jest suite 189/189 across 10 suites.

Automated review 🤖

Summary of Changes

This PR merges in operational hardening around browser/process lifecycle management and memory visibility: it closes Puppeteer browsers on shutdown/init failure, adds periodic process/resource logging, and caps retained in-memory server logs. Overall this should reduce OOM risk and improve diagnosis, with the main runtime change being a new background sampler that shells out to ps.

Key Changes & Positives

• 🟢 (Major) src/builtin-servers/servers/searxng.ts now cancels pending retry timers and calls closeBrowser() during stop() and failed/mid-flight initialization, which directly addresses browser/tab leak scenarios.
• 🟢 (Major) src/services/resourceStats.ts cleanly avoids overlapping samples and awaits in-flight sampling during shutdown, reducing teardown races.
• 🟢 (Minor) src/services/mcp.ts centralizes log retention through appendServerLog() with a clear 500-line cap, which bounds one source of unbounded in-memory growth.
• 🟢 (Minor) Dockerfile adds procps, aligning the container image with the new dependency on ps.
• 🟢 Test coverage is materially improved for lifecycle, log capping, and resource-stat parsing behavior.

Potential Issues & Recommendations

1. • Issue / Risk: src/utils/resourceStats.ts:142 tokenizes ps command= with split(/\s+/), which does not preserve shell quoting/escaping.
   • Impact: shortenProcessLabel() can mis-identify script names for commands containing spaces or quoted arguments, producing misleading labels in logs.
   • Recommendation: If accurate labels matter, prefer sampling comm=/args= separately or treat labels as best-effort and document that quoted command lines are lossy; add a test for quoted script paths to make the limitation explicit.
   • Status: 🟡 Needs review
2. • Issue / Risk: src/services/mcp.ts:934 uses logs.splice(0, logs.length - MAX_SERVER_LOG_LINES) on every append past the cap, which is O(n) per log line.
   • Impact: Very chatty stderr streams could still create avoidable CPU churn even though memory is bounded.
   • Recommendation: If high-volume logging is expected, switch to a ring buffer or trim in larger batches; otherwise keep as-is but note the tradeoff.
   • Status: 🟡 Needs review
3. • Issue / Risk: src/services/resourceStats.ts:107 logs child process names derived from command lines.
   • Impact: Although shortenProcessLabel() avoids many sensitive cases, command-derived labels are still best-effort and may expose executable/script basenames that operators consider sensitive.
   • Recommendation: Confirm this logging is acceptable for your environments, or gate child-name logging behind a debug/ops flag.
   • Status: 🟡 Needs review

Language/Framework Checks

• TypeScript/Node:
  • Types are mostly sound; import type { Resource } in src/services/resourceStats.ts is correct. 🟢
  • Async shutdown handling in src/services/resourceStats.ts and src/builtin-servers/servers/searxng.ts is improved and avoids common timer/promise races. 🟢
  • Runtime validation for RESOURCE_STATS_INTERVAL_MS in src/env.ts is reasonable; non-finite values safely fall back instead of silently disabling. 🟢
  • Dependency bump in package.json/yarn.lock is locked consistently. 🟢
• Docker:
  • Dockerfile:55 correctly adds procps for the new ps runtime dependency. 🟢
  • Not enough context: verify .dockerignore and final image size impact if minimizing image footprint is important.

Security & Privacy

• Child process labels in [resource-stats] logs are derived from process command lines. The shortenProcessLabel() guard reduces secret leakage risk, but this remains worth validating against your logging/privacy requirements.

Build/CI & Ops

• This changes runtime observability and adds periodic ps execution; rollback should consider disabling via RESOURCE_STATS_INTERVAL_MS=0 if sampling causes noise or operational issues.
• The shutdown ordering change in src/index.ts:97 is sensible and lowers the chance of teardown-time logging races.

Tests

• Added tests are strong and target the highest-risk behaviors.
• Recommend one additional unit test for shortenProcessLabel() with quoted/space-containing script paths to document current parsing behavior.

Approval Recommendation

Approve with caveats

• Confirm command-derived child process labels are acceptable in production logs.
• Decide whether the O(n) log-trimming approach is acceptable for expected stderr volume.